Skip to content

Fix ReshapeReshape initializer name collisions when resolving shared -1 shapes#2947

Open
Copilot wants to merge 2 commits into
mainfrom
copilot/bugfix-reshape-reshape-pattern
Open

Fix ReshapeReshape initializer name collisions when resolving shared -1 shapes#2947
Copilot wants to merge 2 commits into
mainfrom
copilot/bugfix-reshape-reshape-pattern

Conversation

Copilot AI commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

ReshapeReshape could rewrite multiple matches that share the same shape initializer ([2, -1]) into distinct concrete shapes, but reuse the same initializer name. This caused initializer map clobbering and left rewritten Reshape inputs pointing to non-registered values after name-fixup.

  • Rewrite rule change (_basic_rules.py)

    • Stop reusing the incoming shape value name for synthesized shape initializers.
    • Generate a per-match initializer name from the matched second reshape node context, then create the new initializer with that unique name.
    • This prevents cross-match overwrite in graph.initializers when multiple chains share one original shape input.
  • Regression coverage (_basic_rules_test.py)

    • Add a focused test with two Reshape -> Reshape chains sharing shared_shape=[2, -1], where -1 resolves differently per chain.
    • Run the same optimizer sub-pipeline used in the repro and assert rewritten reshape shape-inputs are distinct and registered initializers.
# before: both rewrites reused shape.name (e.g. "shared_shape")
new_shape = op.initializer(ir.Tensor(self._new_shape, name=shape.name))

# after: per-match unique shape initializer name
self._new_shape_name = f"{context.nodes[1].name or shape.name}/shape"
new_shape = op.initializer(ir.Tensor(self._new_shape, name=self._new_shape_name))

Copilot AI changed the title [WIP] Fix reshape->reshape pattern bug with -1 values Fix ReshapeReshape initializer name collisions when resolving shared -1 shapes Jun 22, 2026
Copilot AI requested a review from justinchuby June 22, 2026 20:56
@justinchuby justinchuby marked this pull request as ready for review June 23, 2026 17:07
@justinchuby

Copy link
Copy Markdown
Collaborator

@copilot was there a way from the rewrite rule class to get a unique name? No worries if there is not. Just wanted to double check in case there is

Copilot AI commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot was there a way from the rewrite rule class to get a unique name? No worries if there is not. Just wanted to double check in case there is

I double-checked: not from RewriteRuleClassBase today. In rewrite rules, op is a TapeBuilder, and it doesn’t expose a unique-name API for initializers/values. The unique-name helper exists in other contexts (e.g. converter internals), but not through the rewrite-rule interface, so this rule used the matched node context to keep names distinct.

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.66%. Comparing base (5ea1b64) to head (00d3c3a).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2947      +/-   ##
==========================================
+ Coverage   72.64%   72.66%   +0.02%     
==========================================
  Files         259      259              
  Lines       31766    31793      +27     
  Branches     3007     3008       +1     
==========================================
+ Hits        23075    23102      +27     
  Misses       7672     7672              
  Partials     1019     1019              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@titaiwangms titaiwangms requested a review from Copilot June 23, 2026 18:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a correctness issue in the ReshapeReshape rewrite rule where synthesizing new concrete shape initializers could reuse an existing shape initializer name across multiple matches, leading to initializer-map clobbering and invalid value references after subsequent passes.

Changes:

  • Update ReshapeReshape to name synthesized shape initializers using a per-match derived name instead of reusing the incoming shape value’s name.
  • Add a regression test covering two Reshape -> Reshape chains sharing a single shared_shape=[2, -1] initializer where -1 resolves differently per chain.
  • Exercise the optimizer sub-pipeline used in the repro and assert rewritten Reshape shape inputs are distinct and backed by registered initializers.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
onnxscript/rewriter/rules/common/_basic_rules.py Adjusts synthesized shape initializer naming for ReshapeReshape to avoid collisions across multiple matches.
onnxscript/rewriter/rules/common/_basic_rules_test.py Adds regression coverage for shared -1 shape initializer rewrites producing distinct concrete shape initializers.


# Constraints for shape.
self._allowzero = context.nodes[0].attributes.get_int("allowzero", 0)
self._new_shape_name = f"{context.nodes[1].name or shape.name}/shape"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Bug for reshape->reshape pattern which include -1

4 participants